Skip to content

Fix multiple issues uncovered by test suites #641

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 32 commits into from
Mar 17, 2025
Merged

Fix multiple issues uncovered by test suites #641

merged 32 commits into from
Mar 17, 2025

Conversation

ahejlsberg
Copy link
Member

No description provided.

@ahejlsberg ahejlsberg requested a review from jakebailey March 17, 2025 00:43
@@ -1962,6 +1962,16 @@ func TokenToString(token ast.Kind) string {
return tokenToText[token]
}

func GetViableKeywordSuggestions() []string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this the third open PR with this fix 😄

@ahejlsberg
Copy link
Member Author

Most of the fixes here are trivial, but there was an interesting issue with the caching of Relater objects that in border cases allowed old non-zeroed state to survive. They're now kept on a simple free list.

@jakebailey
Copy link
Member

Most of the fixes here are trivial, but there was an interesting issue with the caching of Relater objects that in border cases allowed old non-zeroed state to survive. They're now kept on a simple free list.

Huh, which field did that? I had sliced off or cleared everything, I thought...

@@ -11521,7 +11533,10 @@ func (c *Checker) checkBinaryLikeExpression(left *ast.Node, operatorToken *ast.N
ast.KindGreaterThanGreaterThanGreaterThanEqualsToken:
rhsEval := c.evaluate(right, right)
if numValue, ok := rhsEval.Value.(jsnum.Number); ok && numValue.Abs() >= 32 {
c.errorOrSuggestion(ast.IsEnumMember(ast.WalkUpParenthesizedExpressions(right.Parent.Parent)), errorNode, diagnostics.This_operation_can_be_simplified_This_shift_is_identical_to_0_1_2, scanner.GetTextOfNode(left), scanner.TokenToString(operator), (numValue / 32).Floor())
// Adding 0 removes sign from -0
shiftMod32 := math.Mod(float64(numValue), 32) + 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably just numValue.Remainder(32)? I'd rather not rely on the math package outside of jsnum; I eliminated it everywhere except for the max int consts.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'll switch to Number.Remainder. Which by the way has a bug in it that I'll also fix.

r.c = c
r := c.freeRelater
if r == nil {
r = &Relater{c: c}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I feel like we should probably clean this up into a plain sync pool, but, not in this PR.

@ahejlsberg
Copy link
Member Author

Huh, which field did that? I had sliced off or cleared everything, I thought...

Here's how it went wrong with the old logic: In the first call to checkTypeRelatedToEx, slices.Grow allocates a one-element array. In a following recursive call to checkTypeRelatedToEx, slices.Grow re-allocates a two-element array and copies the current state of the first Relater into the first element of the new two-element array. Later, when the first call returns, it clears the Relater in the old one-element array, but leaves the first element of the two-element array with the copied state, and this state is never cleared.

Yeah, we should switch to a sync pool, but seems like overkill when sync isn't needed.

@ahejlsberg ahejlsberg added this pull request to the merge queue Mar 17, 2025
Merged via the queue into main with commit 6065110 Mar 17, 2025
21 checks passed
@jakebailey jakebailey deleted the more-types-37 branch March 21, 2025 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants